-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shift to Single Checkpoint Files #9333
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
checkpoint_path = chkpt_dir / f"{name}.zip" | ||
|
||
# get a temporary directory to save the checkpoint | ||
temp_dir = Path(tempfile.mkdtemp()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change to tempfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migrations, save api, requires a path to save checkpoint.yaml and checkpoint.blob, we save it in a temp directory and create a zipfile from the temp directory.
The alternative would be to save the checkpoint files (checkpoint.yaml, checkpoint.blob) in the same directory of checkpoints directory, and if multiple notebooks are running at same time, it might cause race conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yashgorana , here instead of persisting a checkpoint directory, we're trying to create a tmp folder to save the data over there and then zip it and make it available as a .zip file under ~/.syft/data/checkpoints/
, so e.g. ~/.syft/data/checkpoints/1-checkpoint.zip
.
Description
The PR shifts checkpoints to single files, for easy portability.
It also adds the ability to load checkpoints from custom paths
Affected Dependencies
List any dependencies that are required for this change.
How has this been tested?
Checklist